Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Card] Adjust padding of Card.Section, footer and header #962

Merged
merged 1 commit into from
Feb 11, 2019
Merged

[Card] Adjust padding of Card.Section, footer and header #962

merged 1 commit into from
Feb 11, 2019
+13 −5

Conversation

elileto
Copy link
Contributor

@elileto elileto commented Feb 4, 2019 •
edited
Loading

WHY are these changes introduced?

Resolves #954

When a subdued section Card and a footer prop were combined there was no padding/margin between the Button Group and the edge of the subdued card.
It was also discovered in the interim there was the same issue with the Header button Group and a subdued section card as well so this will be resolved.

WHAT is this pull request doing?

This request is adding a margin to the footer section.
Before:
image

After:
image

Small screen:
image

With no header and regular section cards:
image

Small screen:
image

Copy-paste this code in playground/Playground.tsx: 
import * as React from 'react';
import {Page, Card, List, Popover, Button, ActionList} from '../src';

interface State {}

export default class Playground extends React.Component<{}, State> {
  render() {
    return (
      <Page title="Playground">
        <Card
          secondaryFooterAction={{content: 'Dismiss'}}
          primaryFooterAction={{content: 'Export Report'}}
        >
          <Card.Header
            actions={[
              {
                content: 'Preview',
              },
            ]}
            title="Staff accounts"
          >
            <Popover
              active
              activator={
                <Button disclosure plain>
                  Add account
                </Button>
              }
              onClose={() => {}}
            >
              <ActionList items={[{content: 'Member'}, {content: 'Admin'}]} />
            </Popover>
          </Card.Header>
          <Card.Section title="Deactivated reports" subdued>
            <List>
              <List.Item>Payouts</List.Item>
              <List.Item>Total Sales By Channel</List.Item>
            </List>
          </Card.Section>
        </Card>
      </Page>
    );
  }
}
without subdued section: 
import * as React from 'react';
import {Page, Card, List, Popover, Button, ActionList} from '../src';

interface State {}

export default class Playground extends React.Component<{}, State> {
  render() {
    return (
      <Page title="Playground">
        <Card
          secondaryFooterAction={{content: 'Dismiss'}}
          primaryFooterAction={{content: 'Export Report'}}
        >
          <Card.Header
            actions={[
              {
                content: 'Preview',
              },
            ]}
            title="Staff accounts"
          >
            <Popover
              active
              activator={
                <Button disclosure plain>
                  Add account
                </Button>
              }
              onClose={() => {}}
            >
              <ActionList items={[{content: 'Member'}, {content: 'Admin'}]} />
            </Popover>
          </Card.Header>
          <Card.Section title="Deactivated reports">
            <List>
              <List.Item>Payouts</List.Item>
              <List.Item>Total Sales By Channel</List.Item>
            </List>
          </Card.Section>
        </Card>
      </Page>
    );
  }
}
### 🎩 checklist

@elileto elileto self-assigned this Feb 4, 2019
@BPScott BPScott temporarily deployed to polaris-react-pr-962 February 4, 2019 16:06 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-962 February 4, 2019 16:09 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-962 February 4, 2019 16:35 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-962 February 4, 2019 17:25 Inactive
@elileto elileto changed the title [WIP][Card] Added top margin to Card Footer [WIP][Card] Add top margin to Card Footer Feb 4, 2019
@elileto elileto mentioned this pull request Feb 4, 2019
Merged
3 tasks
@elileto elileto changed the title [WIP][Card] Add top margin to Card Footer [Card] Add top margin to Card Footer Feb 4, 2019
ry5n
ry5n reviewed Feb 4, 2019
View reviewed changes
Copy link
Contributor

@ry5n ry5n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s possible this might cause too much apparent space when the last section is not subdued. Would it make the CSS too brittle to apply the top margin (and add a border) to the Card footer only when the last section is subdued?

solonaarmstrong-zz
solonaarmstrong-zz reviewed Feb 4, 2019
View reviewed changes
@solonaarmstrong-zz
Copy link

solonaarmstrong-zz commented Feb 4, 2019 •
edited
Loading

Looks 👌

This adds a margin between the footer and a regular section, as well, so I would just make sure that a designer (@ry5n ?) agrees with that decision. It looks better to me with the extra space for both the subdued and regular sections, but I don't know if there was a reason behind the lack of top margin for regular sections.

Ignore me. I just saw Ryan's comment.

Could you try something like:

.Section-subdued:last-of-type ~ .Footer {
  margin-top: spacing(loose);
}

??

@BPScott BPScott temporarily deployed to polaris-react-pr-962 February 6, 2019 13:27 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-962 February 6, 2019 13:28 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-962 February 6, 2019 13:30 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-962 February 6, 2019 13:33 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-962 February 6, 2019 13:36 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-962 February 6, 2019 13:36 Inactive
@elileto elileto changed the title [Card] Add top margin to Card Footer [Card] Adjust padding of Card.Section, footer and header Feb 6, 2019
@elileto elileto changed the title [Card] Adjust padding of Card.Section, footer and header [WIP][Card] Adjust padding of Card.Section, footer and header Feb 6, 2019
@elileto elileto changed the title [WIP][Card] Adjust padding of Card.Section, footer and header [Card] Adjust padding of Card.Section, footer and header Feb 6, 2019
@BPScott BPScott temporarily deployed to polaris-react-pr-962 February 6, 2019 15:34 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-962 February 6, 2019 15:35 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-962 February 8, 2019 03:54 Inactive
ry5n
ry5n suggested changes Feb 8, 2019
View reviewed changes
ry5n
ry5n reviewed Feb 8, 2019
View reviewed changes
ry5n
ry5n reviewed Feb 8, 2019
View reviewed changes
@BPScott BPScott temporarily deployed to polaris-react-pr-962 February 8, 2019 04:06 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-962 February 8, 2019 04:06 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-962 February 8, 2019 04:15 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-962 February 8, 2019 04:17 Inactive
@elileto elileto requested a review from ry5n February 8, 2019 20:43
@BPScott BPScott temporarily deployed to polaris-react-pr-962 February 11, 2019 14:31 Inactive
ry5n
ry5n approved these changes Feb 11, 2019
View reviewed changes
Copy link
Contributor

@ry5n ry5n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks for your hard work on this 🎉

@solonaarmstrong-zz
Copy link

👌

@elileto elileto merged commit bdcbb7c into master Feb 11, 2019
@elileto elileto deleted the Card-padding branch February 11, 2019 21:13
@danrosenthal danrosenthal temporarily deployed to production February 11, 2019 21:49 Inactive
elileto added a commit that referenced this pull request Feb 20, 2019
@elileto
Revert "Merge pull request #962 from Shopify/Card-padding"
540a325 
This reverts commit bdcbb7c, reversing
changes made to 8c80975.
@elileto elileto restored the Card-padding branch February 20, 2019 22:41
@kaelig kaelig deleted the Card-padding branch February 21, 2019 00:49
@danrosenthal danrosenthal mentioned this pull request Feb 21, 2019
Merged
@solonaarmstrong-zz solonaarmstrong-zz mentioned this pull request Feb 21, 2019
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ry5n ry5n ry5n approved these changes

@solonaarmstrong-zz solonaarmstrong-zz solonaarmstrong-zz approved these changes

Assignees

@elileto elileto

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Card] Padding issue between footer actions and subdued section card
5 participants
@elileto @solonaarmstrong-zz @ry5n @BPScott @danrosenthal